Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed MaxStackSize setter. #892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kekyo
Copy link

@kekyo kekyo commented Jan 19, 2023

When generating assemblies using cecil, I manually specified .maxstack for MethodBody, but it is not reflected. It is always overwritten with the calculated result. This problem has been fixed in this PR.

Example:

var method = new MethodDefinition(...);
var body = method.Body;

body.MaxStackSize = 100;

// ...

module.Write("foo.dll");

Comment on lines 344 to 346
if (body.max_stack_size == 0) {
body.max_stack_size = max_stack;
}
Copy link
Contributor

@ltrzesniewski ltrzesniewski Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CodeReader sets max_stack_size to a nonzero value for methods with a fat header:

body.max_stack_size = ReadUInt16 ();

This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user.

The following would be a safer alternative:

Suggested change
if (body.max_stack_size == 0) {
body.max_stack_size = max_stack;
}
body.max_stack_size = Math.Max(body.max_stack_size, max_stack);

But then, changes that decrease the max stack size wouldn't be taken into account automatically. So maybe you need a "maxstack is set explicitly" flag.

Also, note this only applies to fat headers. Cecil will always write a tiny header if possible. I'm actually curious why you need this change in the first place.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm writing an assembler tooling and using cecil for output. One time I noticed that the value of .maxstack output by cecil sometimes differed from the expected value.

As it turns out, the problem was a mistake in my assembler code, but it made me think a bit about calculating the value of maxstack. For example, which is a rather aggressive:

int32 foo(int32 count)
{
  .locals ([0] int32 i)

  ldarg.0   ; i = count
  stloc.0

L1:
  ldc.i4.1  ; [increase ev stack]

  ldloc.0   ; i--
  ldc.i4.1
  sub
  stloc.0

  ldloc.0   ; if (i != 0) goto L1
  ldc.i4.0
  ceq
  brfalse L1

  ; (snip: decrease ev stack by count)

  ret
}

Even if a static execution flow analysis is performed, complete maxstack size cannot be calculated in advance. Therefore, the person writing the assembly code must manually set an appropriate size for maxstack.

Of course in most cases, the size should be set to the size obtained from the static execution flow analysis, so I believe we only need to deal with this exceptional situation. So, I explicitly specified a value for the MaxStackSize property only if a size was specified, but this was not reflected.

This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user.

I completely agree and it was my mistake that the existing implementation was affected. May I make additional changes? I will use the "maxstack is set explicitly" flag as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complete maxstack size cannot be calculated in advance

Oh, you're misunderstanding what maxstack means. It's not the max size of the execution stack, but the max size of the IL evaluation stack. The IL evaluation stack size is constant for each IL instruction.

See ECMA-335 III.1.7.4:

[Note: Maxstack is related to analysis of the program, not to the size of the stack at runtime. It does not specify the maximum size in bytes of a stack frame, but rather the number of items that
shall be tracked by an analysis tool. end note]

[Rationale: By analyzing the CIL stream for any method, it is easy to determine how many items
will be pushed on the CIL Evaluation stack. However, specifying that maximum number ahead
of time helps a CIL-to-native-code compiler (especially a simple one that does only a single pass
through the CIL stream) in allocating internal data structures that model the stack and/or
verification algorithm. end rationale]

TL;DR: It can always be calculated statically, and Cecil does the right thing. 🙂

May I make additional changes?

I'm not the maintainer, just a guy on the internet trying to help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the maintainer, just a guy on the internet trying to help.

Thanks for reviewing my code! I await comments from the maintainer.

I understand the difference between the runtime stack (as indicated by rsp and rbp in x64) and the evaluation stack. And I also understand that the topic about maxstack is about the evaluation stack.

The sample code indicates that "If the max stack size of the evaluation stack changes at "run-time", max stack size cannot be calculated by static analysis."

I could not read the meaning from the description in the ECMA specification that the evaluation stack consumption is always statically determined. In fact, if we write code like the sample code above, we will see that it is not possible to estimate it in advance. And it causes InvalidProgramException when maxstack is insufficient.

I thought there might be a need to explicitly specify maxstack when outputting assemblies.

Copy link
Contributor

@ltrzesniewski ltrzesniewski Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. If you're trying to write custom tooling (something that replaces the .NET JIT), then this makes sense. Your code could accept a variable evaluation stack depth.

Just be aware this is not correct ECMA-335 IL though, and is not accepted by the .NET JIT. You could support that as a custom extension to the standard.

Here's an example project: ConsoleApp.zip, it does an inifite loop while pushing an item to the stack. The .NET JIT immediately rejects it with an InvalidProgramException.

See the III.1.7.5 paragraph from ECMA-335 which defines the related correctness rule (emphasis mine):

It shall be possible, with a single forward-pass through the CIL instruction stream for any
method, to infer the exact state of the evaluation stack at every instruction (where by “state” we
mean the number and type of each item on the evaluation stack).

In particular, if that single-pass analysis arrives at an instruction, call it location X, that
immediately follows an unconditional branch, and where X is not the target of an earlier branch
instruction, then the state of the evaluation stack at X, clearly, cannot be derived from existing
information. In this case, the CLI demands that the evaluation stack at X be empty.

Following on from this rule, it would clearly be invalid CIL if a later branch instruction to X
were to have a non-empty evaluation stack

[Rationale: This constraint ensures that CIL code can be processed by a simple CIL-to-nativecode compiler. It ensures that the state of the evaluation stack at the beginning of each CIL can
be inferred from a single, forward-pass analysis of the instruction stream. end rationale]

[Note: the stack state at location X in the above can be inferred by various means: from a
previous forward branch to X; because X marks the start of an exception handler, etc. end note]

The rules for merging evaluation stack frames described in III.1.8.1.3 could also be relevant, even though they determine verifiability (and not correctness).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for showing the pointer. I referred to the text of ECMA, as it seems I only looked at the quoted part and misinterpreted it.

And I understood that in III.1.7.4 and III.1.7.5, the statically analyzed maxstack must be set and its value must be correct.
I now understand that this means that an instruction stream like the sample code I showed can be written, but it is not guaranteed (nor should it be) that the CLR can execute it. Your point is absolutely correct!!! 😄 If someone asks me, I can now explain to them that they should not write such CIL code.

So, back to cecil, I always feel that the MaxStackSize property should not have a setter, since the maxstack can be calculated by static analysis. The CoreReader stores the value directly in the backing store of the MaxStackSize property. So without a setter, cecil users will be able to treat MaxStackSize as a view in the true sense.

I pushed the code modified to do so. Thank you explain it real issue again!

@kekyo kekyo force-pushed the fix-maxstack-ignored branch from 40c55c1 to 5debb46 Compare January 20, 2023 02:49
@kekyo
Copy link
Author

kekyo commented Jan 20, 2023

I wasn't sure if the setter for the MaxStackSize property should check to see if the value has been changed, but I determined that the intention was to change it when the setter was called. How about this?

@kekyo kekyo force-pushed the fix-maxstack-ignored branch from 5debb46 to e7e5bb8 Compare January 21, 2023 02:54
@kekyo kekyo changed the title Fixed explicitly applied maxstack being ignored. Removed MaxStackSize setter. Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants